-
Notifications
You must be signed in to change notification settings - Fork 9
Add the multiAddress.enabled cluster option present in zk >3.6.0 #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Jin Huang <[email protected]>
This reverts commit 47fdb51. Not needed internally
* Make zookeeper ids start from an offset * Bootstrap zookeeper cluster from an external SEED_NODE * Use public ip for internal quorum election * Delay zookeeper start to allow for LB registration * Fix a bug when a node is teared down * Implement review
… is not available in spec
- k8s libs to v0.27.5 - controller-runtime to v0.15.0 Updated usage for fake client - now required WithStatusSubresource(..) MultiNamespacedCacheBuilder is deprecated switched to usage Cache.Options.Namespaces. Updated usage for depricated wait.Poll and use fake.NewClientBuilder() Fixes: pravega#567 Signed-off-by: Ann Taraday <[email protected]> Change-Id: Ia673486d8ebb3c76e86d1cc3af846cd890b4ffaa
…d up by the deployments
…ookeeperReady functionality in progress
docker/zookeeper-image/zkServer.sh
Outdated
|
|
||
| # | ||
| # added -Dzookeeper.multiAddress.enabled=true option to allow for multiple | ||
| # zookeeper addresses in the zookeeper-operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we avoid duplicating the upstream script and instead leverage SERVER_JVMFLAGS env var to pass in -Dzookeeper.multiAddress.enabled=true ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use something similar internally at https://git.corp.adobe.com/experience-platform/pipeline-kafka/blob/master/k8s/charts/zookeeper-cluster/values.yaml#L112-L113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deploying with the SERVER_JVMFLAGS works. Removed this file and now theres no longer a need for a new apache image as well.
I noted the pod env requirement in a comment in generators.go. Should I add a section to the README about multiaddress deployment configuration as well?
| echo "Extra server addresses present" | ||
| ORIGINALADDRESS=${ZKCONFIG%"$suffix"} | ||
| echo "server.${MYID}=${ORIGINALADDRESS}|${EXTRACONFIG}" > $DYNCONFIG | ||
| else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we haven't used this before - is this required for cases where EXTRACONFIG is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not. When you run zkconfig it sends out a format that is address:quorum-port:leade-port:role;client-port which follows the zk formatting for addresses. When you have multiple addresses the leader and quorum ports need to be duplicated, the role is optional but you can only have ";client-port" once at the end of the line. So if there is no extra address address:<quorum-port:leader-port:role;client--port works, but adding that extra address we need to trim off the ;client-port in the first one so we have address1:quorum-port:leader-port:role|address2:quorum-port:leader-port:role;client--port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the doc on multiple address formatting: https://zookeeper.apache.org/doc/current/zookeeperReconfig.html#sc_multiaddress
While it doesn't explicitly say the client port can't be duplicated, in development this caused an error that was something to the effect of "address format invalid, should be host:port:port|host:port:port;port..."
…ess enabled flag in a server_jvmflag for the pod vars
docker/zookeeper-image/Dockerfile
Outdated
|
|
||
| # overwrite default zkserver.sh in here to add multiaddress.enabled | ||
| RUN rm /$DISTRO_NAME/bin/zkServer.sh | ||
| COPY zkServer.sh /$DISTRO_NAME/bin/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this zkServer.sh coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was left over from before you pointed out that I could set multiAddress.enabled with a jvm flag. I just removed this.
166e2b5 to
391a8c1
Compare
d0623be to
a4c9894
Compare
0061d79 to
5d74c24
Compare
Change log description
(2-3 concise points about the changes in this PR. When committing this PR, the committer is expected to copy the content of this section to the merge description box)
Purpose of the change
In zookeeper > 3.6.0 you can specify multiple addresses for a singular zookeeper server. here is the description of that cluster option. This update allows you to leverage that feature through the operator. This can also be useful if you want to expose individual zk servers through external addresses.
(e.g., Fixes #666, Closes #1234)
What the code does
(Detailed description of the code changes)
How to verify it
You can add addresses for some, all or no servers. You can also add multiple addresses for the same server in a pipe separated list.